Skip to content

Avoid logging AuthToken from transition links#5425

Merged
TomatoToaster merged 1 commit into
mainfrom
amal-dont-log-token
Sep 22, 2021
Merged

Avoid logging AuthToken from transition links#5425
TomatoToaster merged 1 commit into
mainfrom
amal-dont-log-token

Conversation

@TomatoToaster

@TomatoToaster TomatoToaster commented Sep 22, 2021

Copy link
Copy Markdown
Contributor

CC: @marcaaron

Details

Fixed Issues

Follow up to a problem identified here: #5396 (comment)

Tests

Same as Web QA done locally

QA Steps

Exact same as this PR: #5396
After you've navigated to NewDot successfully, open up the JS console and search for the log line: Navigating from transition link from OldDot using short lived authToken and verify you don't see any huge authToken in the logs like here: #5396 (comment)

image

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

N/A

@TomatoToaster TomatoToaster requested a review from a team as a code owner September 22, 2021 19:59
@TomatoToaster TomatoToaster self-assigned this Sep 22, 2021
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@MelvinBot MelvinBot requested review from robertjchen and removed request for a team September 22, 2021 20:00
@TomatoToaster

Copy link
Copy Markdown
Contributor Author

Tested this locally and it works:
image

@marcaaron marcaaron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


// Don't log the route transitions from OldDot because they contain authTokens
if (path.includes('/transition/')) {
Log.info('Navigating from transition link from OldDot using short lived authToken');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't think we need to log this immediately though so we can add a false like we are doing in the other call?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually wait theres no need, it's false by default: https://github.com/Expensify/expensify-common/blob/master/lib/Logger.jsx#L82

@TomatoToaster TomatoToaster merged commit 64e2140 into main Sep 22, 2021
@TomatoToaster TomatoToaster deleted the amal-dont-log-token branch September 22, 2021 21:04
github-actions Bot pushed a commit that referenced this pull request Sep 22, 2021
Avoid logging AuthToken from transition links

(cherry picked from commit 64e2140)
@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by @TomatoToaster in version: 1.1.1-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@TomatoToaster

Copy link
Copy Markdown
Contributor Author

My bad on the testing steps, it's not actually going to be on the dev console in staging, but in the real logs. @MitchExpensify tested it and the logs for the request being correctly logged are right here: https://www.expensify.com/_devportal/tools/logSearch/#sort=asc&size=2000&query=blob%3A%20%22Navigating%20from%22%20AND%20email%3A%22mitch%40frankchu.xyz%22%20AND%20timestamp%3A%5B2021-09-23T00%3A00%20TO%202021-09-24T23%3A59%5D

So i can say this passed QA on Staging 👍🏾

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.1-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @TomatoToaster in version: 1.1.1-9 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @timszot in version: 1.1.2-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants